-
Notifications
You must be signed in to change notification settings - Fork 602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: upgrade to react 18 - fix build #2 #3774
feat: upgrade to react 18 - fix build #2 #3774
Conversation
While working on this batch I have encountered some issue with build for |
Yes, you can update the rmwc. Probably make it as separate PR because there will be a lot of changes. |
@@ -15,24 +16,29 @@ export interface RequireNewPassword { | |||
requiredAttributes: string[]; | |||
} | |||
|
|||
type CheckContactParams = { | |||
verified: Record<any, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please figure out some better types. We cannot trust what you currently wrote.
@@ -21,6 +22,11 @@ interface Reducer { | |||
(prev: State, next: Partial<State>): State; | |||
} | |||
|
|||
type CheckContactParams = { | |||
verified: Record<any, any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please figure out better types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunozoric improved Record<any, any>
type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neatbyte-ivelychko putting object
to AuthData
interface instead of Record<any, any> is same bad type written in another way.
The verified
is not used anywhere except two checks if it actually is defined, so maybe try to figure out what is contained in it? Don't spend much more time on it. If you can't find the correct type, put
{verified?: Record<string, string | number | boolean>}
for now.
Also, I see that unverified is not used anywhere, so maybe it is not necesary at the moment...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But still we need to add object
in the AuthData
type [key: string]: string | null | boolean | undefined | object;
property. Becuase TS keeps arguing that that type does not support object, it only supports string | null | boolean | undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When logging the data variable, I get:
{
"verified": {
"email": "my@email.com"
},
"unverified": {}
}
Maybe it could be like this?
export interface AuthDataVerified {
email?: string;
}
export interface AuthDataUnverified {
someUnknownKey?: string;
}
export interface AuthData {
username?: string;
email?: string;
verified?: AuthDataVerified;
unverified?: AuthDataUnverified;
[key: string]: string | null | boolean | undefined | AuthDataVerified | AuthDataUnverified;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have also checked method verifiedContact
in @aws-amplify
and I saw that besides email
property they have phone_number
property. Should we consider adding it to the verified
and unverified
.
if (attrs['email']) {
if (attrs['email_verified']) {
verified['email'] = attrs['email'];
} else {
unverified['email'] = attrs['email'];
}
}
if (attrs['phone_number']) {
if (attrs['phone_number_verified']) {
verified['phone_number'] = attrs['phone_number'];
} else {
unverified['phone_number'] = attrs['phone_number'];
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neatbyte-ivelychko yes, add anything you can find. Maybe we don't use those vars now, but let's have them in types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added phone_number
to the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brunozoric I think that this might be the latest batch. Because after I have fixed type for ComposableFC project had been built without any errors.
We just need to check whether it is gonna pass CI/CD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, it was the issue with my local build. Somehow it was always green event if build had errors
df50e32
to
9ca6617
Compare
9ca6617
to
69098ee
Compare
85f5c3f
to
52f7308
Compare
886860a
into
bruno/feat/upgrade-to-react-18
Changes
First batch of fixes for project build after updating to react 18.
https://www.notion.so/webiny/Upgrade-to-React-18-ceacd9bbfcf74b39a7ceb70943c3afa7
Packages: app-cognito-authenticator, react-router, ui
How Has This Been Tested?
Manually.
Documentation
None.